🐛 fix(deploy): reflect intercepted 403/404 from the ingress error backend instead of answering 503 - #17432

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign hanthor for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

ℹ️ Sentinel notice — informational; author is trusted (owner), merge is not blocked

Hive flagged this PR (author @clubanderson, head eb92f68c740e) because it matches behaviors that can override security controls, escalate privileges or damage the codebase. This is a heuristic, not an accusation — a maintainer should confirm the change is intended before it merges.

• sensitive_path — changes 1 sensitive path(s) (Touches a sensitive path (OWNERS, workflows, policies, hive config, security docs, …))
  • src/deploy/k8s/error-pages.yaml

Hive did not add the sentinel-alert label because this author is trusted. The finding remains recorded for audit and the dashboard Security tab; set sentinel.trusted_authors_block: true to require the blocking label for trusted authors too.

Quality note: this is a behaviour change to the error backend, and src/pkg/hub/error_pages_backend_test.go already pins its siblings (#4241 return 5xx, JSON-for-API, healthz). The 403/404 branch has no pin, so the next edit to default.conf can drop error_page 403 404 and nothing goes red — the Soft-404 would come back silently.

A test in the same file, using the existing helpers, that fails on v5 and passes with this PR:

// TestErrorBackendReflects403And404 pins #17431: an intercepted 403/404 must be
// answered with that status and a document that tells no one to "retry
// shortly" — no auto-refresh, and noindex so search engines do not record a
// Soft 404 against a healthy hub.
func TestErrorBackendReflects403And404(t *testing.T) {
	docs, conf := errorPagesConfigMaps(t)
	directives := stripNginxComments(conf["default.conf"])

	for _, code := range []string{"403", "404"} {
		if !regexp.MustCompile(`return\s+` + code + `\b`).MatchString(directives) {
			t.Errorf("default.conf never returns %s, so an intercepted %s falls through to the 503 page (#17431)", code, code)
		}
	}
	if !regexp.MustCompile(`error_page\s+403\s+404\s+/notfound\.`).MatchString(directives) {
		t.Error("default.conf does not route 403/404 to the notfound document (#17431)")
	}

	body, ok := docs["notfound.json"]
	if !ok {
		t.Fatal("hive-error-pages ConfigMap has no notfound.json for API callers")
	}
	var parsed map[string]any
	if err := json.Unmarshal([]byte(body), &parsed); err != nil {
		t.Fatalf("notfound.json is not valid JSON: %v", err)
	}
	if parsed["ok"] != false {
		t.Errorf(`notfound.json "ok" = %v, want false`, parsed["ok"])
	}

	html, ok := docs["notfound.html"]
	if !ok {
		t.Fatal("hive-error-pages ConfigMap has no notfound.html")
	}
	lower := strings.ToLower(html)
	if !strings.Contains(lower, `name="robots" content="noindex"`) {
		t.Error("notfound.html is indexable; Search Console will record a Soft 404 again (#17431)")
	}
	if strings.Contains(lower, "http-equiv=\"refresh\"") || strings.Contains(lower, "settimeout") || strings.Contains(lower, "location.reload") {
		t.Error("notfound.html auto-refreshes; a missing page is not a 'retry shortly' state")
	}
}

No push from this lane (your branch) — adding it to this PR keeps the pin and the fix in one commit.

🐝 Hive Agent: quality | Instance: hosted-available-oke-11-placeholder-r05x | SHA: unknown

— hive: agent=quality backend=copilot model=claude-fable-5.1 copilot=1.0.88

Review follow-ups (below review.backlog.min_severity: P2)

Non-blocking findings below the backlog filing line are listed here instead of as issues, one comment per PR, updated in place. Tick them off as they are fixed; set review.backlog.min_severity: P3 to file every finding.

• P3 info style — notfound.html diverges cosmetically from sibling documents (src/deploy/k8s/error-pages.yaml:92): notfound.html styles links #58a6ff (error-pages.yaml:92) where unauthorized.html uses the brand amber #f0b429 (error-pages.yaml:140), and reuses the .bee class for a 🔍 glyph (error-pages.yaml:9…
• P3 low docs-currency — Global custom-http-errors guidance lives only in an nginx comment, not in an operator doc (src/deploy/k8s/error-pages.yaml:167): The PR records the operational rule 'Keep that global list to "502,503,504"' for the ingress-nginx controller ConfigMap inside the default.conf header comment (src/deploy/k8s/error-pages.yaml:165-171…

Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor.

Review findings from adjudicator (folded here because this head already carries its hive review):

Correctness review (read at head 391d212, which superseded the dispatched eb92f68).

The nginx mechanics are sound: error_page 403 404 /notfound.$hive_error_ext (src/deploy/k8s/error-pages.yaml:154) preserves the return status, the internal locations at :159-160 are reachable via the internal redirect, and the html volume mounts the whole hive-error-pages ConfigMap with no items: (:299-301), so notfound.{html,json} land on disk. I verified nothing regresses the 401/5xx paths.

1. The new branches are the only ones in this backend without a pinning test (medium)

Every other status this backend reflects is guarded: TestErrorBackendNeverAnswers200 pins return 502/503 (src/pkg/hub/error_pages_backend_test.go:110-114), TestErrorBackendServesJSONToAPICallers pins error.json as valid JSON with ok:false (:148-165), and TestErrorBackendServes401AsJSON pins error_page 401, both documents, and the error key (src/pkg/hub/saas_api_xhr_ingress_test.go:161-192). Nothing pins return 403/return 404 (error-pages.yaml:173-174), the error_page 403 404 mapping, or that notfound.json (:67) is valid JSON with ok:false — so the contract of #17431 ("an intercepted 404 can never again become a 503") can regress silently, and a typo in notfound.json would hand API callers an unparseable body. Mirror of the 401 test, appended to error_pages_backend_test.go:

+// TestErrorBackendReflects404NotAs503 pins #17431: an intercepted 403 or 404
+// must come back as that status with the not-found documents, never as the
+// 503 "restarting" page that Search Console reported as a Soft 404.
+func TestErrorBackendReflects404NotAs503(t *testing.T) {
+	docs, conf := errorPagesConfigMaps(t)
+	directives := stripNginxComments(conf["default.conf"])
+	for _, code := range []string{"403", "404"} {
+		if !regexp.MustCompile(`return\s+` + code + `\b`).MatchString(directives) {
+			t.Errorf("default.conf never returns %s; an intercepted %s falls through to 503 (#17431)", code, code)
+		}
+	}
+	if !regexp.MustCompile(`error_page\s+403\s+404\s+/notfound\.\$hive_error_ext`).MatchString(directives) {
+		t.Error("default.conf does not map 403/404 onto the notfound.{json,html} documents")
+	}
+	for _, name := range []string{"notfound.json", "notfound.html"} {
+		if _, ok := docs[name]; !ok {
+			t.Errorf("hive-error-pages ConfigMap has no %s", name)
+		}
+	}
+	var parsed map[string]any
+	if err := json.Unmarshal([]byte(docs["notfound.json"]), &parsed); err != nil {
+		t.Fatalf("notfound.json is not valid JSON: %v", err)
+	}
+	if parsed["ok"] != false {
+		t.Errorf(`notfound.json must report "ok": false, got %v`, parsed["ok"])
+	}
+	if html := docs["notfound.html"]; strings.Contains(html, `http-equiv="refresh"`) || !strings.Contains(html, `name="robots" content="noindex"`) {
+		t.Error("notfound.html must not auto-refresh and must carry noindex (Soft 404, #17431)")
+	}
+}

2. Advisory: a reflected 403 tells API callers "error":"not found" (low)

notfound.json (error-pages.yaml:67) is served for both 403 and 404 (:154). The upstream's 403 body — e.g. the "unmanaged repo" JSON the header comment at :122-124 describes — is replaced with "error":"not found", so a client keying off error cannot tell forbidden from missing. Per #17431 the shared document is the intended design and 403 is no longer in the global list, so this only matters if the list grows again; not blocking. If you want it exact, a one-line forbidden.json plus error_page 403 /forbidden.$hive_error_ext; is enough.

merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at 391d212.

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback